Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable target_feature on any LLVM 6+ #49428

Merged
merged 1 commit into from
Mar 29, 2018
Merged

Enable target_feature on any LLVM 6+ #49428

merged 1 commit into from
Mar 29, 2018

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Mar 27, 2018

In LLVMRustHasFeature(), rather than using MCInfo->getFeatureTable()
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

/// Check whether the subtarget features are enabled/disabled as per
/// the provided string, ignoring all other features.
bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have target_feature.

r? @alexcrichton

@cuviper
Enable target_feature on any LLVM 6+
a93a4d2 
In `LLVMRustHasFeature()`, rather than using `MCInfo->getFeatureTable()`
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

    /// Check whether the subtarget features are enabled/disabled as per
    /// the provided string, ignoring all other features.
    bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have `target_feature`.
@cuviper cuviper added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2018
@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

(The bot seems to be asleep, so I've manually set the requested reviewer...)

@hanna-kruppe
Copy link
Contributor

Does this mean we can drop rust-lang/llvm@a8a60a1?

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

@rkruppe those tables are still used for rustc --print target-cpus and --print target-features, but I'm less worried about those since they're just informative.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

(An alternative for that same info is llc -mcpu=help.)

@hanna-kruppe
Copy link
Contributor

Surely --print target-features should use rustc's whitelist (and differing names)? Good point about --print target-cpus though.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

I think --print target-features is meant to help feed -C target-feature, which operates at the LLVM level. So I'm not sure the whitelist and differing names are applicable.

@hanna-kruppe
Copy link
Contributor

Ugh, that's news to me. IMO the -C option should match the attributes, including whitelisting and renaming. But that's beyond the scope of this PR.

@alexcrichton
Copy link
Member

@bors: r+

Awesome, thanks so much @cuviper!

@bors
Copy link
Contributor

bors commented Mar 27, 2018

📌 Commit a93a4d2 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Mar 28, 2018
@kennytm
Rollup merge of rust-lang#49428 - cuviper:llvm6-target_feature, r=ale…
294f041 
…xcrichton

Enable target_feature on any LLVM 6+

In `LLVMRustHasFeature()`, rather than using `MCInfo->getFeatureTable()`
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

    /// Check whether the subtarget features are enabled/disabled as per
    /// the provided string, ignoring all other features.
    bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have `target_feature`.

r? @alexcrichton
@kennytm kennytm mentioned this pull request Mar 28, 2018
Merged
bors added a commit that referenced this pull request Mar 28, 2018
@bors
Auto merge of #49460 - kennytm:rollup, r=kennytm
d52c44e 
Rollup of 12 pull requests

- Successful merges: #49243, #49329, #49364, #49400, #49405, #49427, #49428, #49429, #49439, #49442, #49444, #49452
- Failed merges:
@bors bors merged commit a93a4d2 into rust-lang:master Mar 29, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 4, 2018 •
edited
Loading

@rkruppe the problem is that -C target-feature has been available on stable since forever and has always mapped to LLVM directly, so changing that was considered a backwards incompatible change. The target feature RFC added -C feature-enable=... (or something like that) to map to Rust target features but it is not implemented yet.

@hanna-kruppe hanna-kruppe mentioned this pull request Apr 4, 2018
Open
@hanna-kruppe
Copy link
Contributor

@gnzlbg let's hash this out in #49653

@cuviper cuviper deleted the llvm6-target_feature branch April 17, 2018 18:42
@semarie semarie mentioned this pull request May 15, 2018
Closed
@cuviper cuviper mentioned this pull request Jun 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton Awaiting requested review from alexcrichton

Assignees
No one assigned
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cuviper @hanna-kruppe @alexcrichton @bors @gnzlbg